Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use an EnvironmentTarget helper instead of direct isinstance checks #21586

Merged
merged 1 commit into from
Oct 29, 2024
Merged

use an EnvironmentTarget helper instead of direct isinstance checks #21586

merged 1 commit into from
Oct 29, 2024

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Oct 29, 2024

A user reported that Pants was generating a warning for the visibility lint rules because the target was running in an experimental_workspace_environment. As it turns out, several parts of the code were checking EnvironmentTarget.val directly to see if it was an instance of LocalEnvironmentTarget. This is an anti-pattern given that we can introduce new environment types.

This PR fixes the issue by using an existing helper method on EnvironmentTarget. The helper method is renamed to can_access_local_system_paths from can_use_system_path_metadata_requests.

@tdyas tdyas added category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required PR doesn't require mention in release notes labels Oct 29, 2024
@tdyas tdyas modified the milestone: 2.23.x Oct 29, 2024
cognifloyd
cognifloyd approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@cognifloyd
Copy link
Member

Why did you decide not to cherry-pick to 2.23? Wait till after 2.23.0 (so cherry pick for 2.23.1)?

@tdyas
Copy link
Contributor Author

tdyas commented Oct 29, 2024

Why did you decide not to cherry-pick to 2.23? Wait till after 2.23.0 (so cherry pick for 2.23.1)?

Because this PR won't apply cleanly to 2.23.x. I likely need to back port the commit which added the helper in the first place.

@tdyas tdyas merged commit f7daf8f into pantsbuild:main Oct 29, 2024
24 checks passed
@tdyas tdyas deleted the use_env_tgt_helper branch October 29, 2024 22:01
@benjyw
Copy link
Contributor

benjyw commented Oct 29, 2024

Why cherry-pick at all? Seems fine not to? This isn't really a user-facing bugfix AFAICT?

@tdyas
Copy link
Contributor Author

tdyas commented Oct 29, 2024

Why cherry-pick at all? Seems fine not to? This isn't really a user-facing bugfix AFAICT?

Users may be confused by the warning if using a workspace environment and some linters as @cognifloyd saw. But to your point, I don't expect a lot of people to hit this so probably can avoid fixing for now.

@cognifloyd
Copy link
Member

This is a pretty minor cosmetic issue (an extra warning), so a cherry-pick could have been nice if it was an easily backported fix. It's not, however, so the cherry-pick is not worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cognifloyd cognifloyd cognifloyd approved these changes

@benjyw benjyw Awaiting requested review from benjyw

Assignees
No one assigned
Labels
category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required PR doesn't require mention in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tdyas @cognifloyd @benjyw